Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[15.0][MIG] base_tier_validation_waiting #638

Closed
wants to merge 14 commits into from

Conversation

bosd
Copy link
Contributor

@bosd bosd commented Apr 19, 2023

Depends on:
#732 Do not sent double messages on the first validation sequence

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch from 05371d2 to 1084cad Compare April 19, 2023 12:48
@bosd
Copy link
Contributor Author

bosd commented Apr 19, 2023

/ocabot migration base_tier_validation_waiting

@OCA-git-bot
Copy link
Contributor

Sorry @bosd you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@bosd bosd mentioned this pull request Apr 19, 2023
34 tasks
@bosd bosd marked this pull request as ready for review April 19, 2023 12:53
@simahawk
Copy link
Contributor

/ocabot migration base_tier_validation_waiting

@OCA-git-bot OCA-git-bot added this to the 15.0 milestone Apr 19, 2023
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG minor comments

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch 4 times, most recently from 097e6d1 to 26c2caa Compare April 20, 2023 09:23
@bosd bosd requested a review from simahawk April 20, 2023 09:28
@bosd
Copy link
Contributor Author

bosd commented Jun 22, 2023

@cwjoalder Can you please review?

@bosd
Copy link
Contributor Author

bosd commented Jun 22, 2023

@simahawk Can you review the changes? 🙏

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch from 81eca93 to 26c2caa Compare August 12, 2023 20:53
@bosd
Copy link
Contributor Author

bosd commented Aug 12, 2023

Force pushed to recreate runboat

@bosd bosd requested a review from michaelslade54 October 7, 2023 11:57
@bosd
Copy link
Contributor Author

bosd commented Oct 7, 2023

@Dranyel-Bosd Can you please review?

Copy link

@Dranyel-Bosd Dranyel-Bosd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks so good to me 👍 🎇

if record.status != "waiting":
continue
# if approve by sequence, check sequence has been reached
if record.approve_sequence:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get this IF block: in any case you set "pending" -> can't you have 1 single check?

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch 3 times, most recently from 7d8edf5 to 3c5d1ce Compare October 23, 2023 06:59
@bosd
Copy link
Contributor Author

bosd commented Oct 23, 2023

@simahawk Thanks for your review!

I'm working my way towards more coverage and green tests 😁
Adding coverage by testing the notifications.
But it's breaking/affecting tests of other modules.
Added the fixes for those in this PR. Is that allowed/correct OCA procedure?

I've watched your talks about the unittests.
Still I'm struggling with getting how to get the tier_validation_server_action fixed.

This module is altering the behaviour by adding the state waiting, which results in breaking the tests of server action.
When this module is installed, an alternative version of tests should be run by that module. Is that correct?
Can you explain how to do that? 😊 🙏


===================================================================== 
2023-10-23 07:04:27,754 1037 ERROR odoo odoo.addons.base_tier_validation_server_action.tests.test_tier_validation: FAIL: TierTierValidation.test_1_auto_validation
Traceback (most recent call last):
  File "/__w/server-ux/server-ux/base_tier_validation_server_action/tests/test_tier_validation.py", line 100, in test_1_auto_validation
    self.assertEqual(
AssertionError: Lists differ: ['waiting', 'waiting', 'waiting'] != ['pending', 'pending', 'pending']

First differing element 0:
'waiting'
'pending'

- ['waiting', 'waiting', 'waiting']
+ ['pending', 'pending', 'pending']
 

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch 2 times, most recently from 9b63c33 to 2f5281f Compare October 25, 2023 04:45
@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch from 2f5281f to f68e73c Compare November 2, 2023 21:35
@DavidJForgeFlow
Copy link

from odoo import api, fields, models
from odoo.tools import config


class TierReview(models.Model):
    _inherit = "tier.review"


    def _default_status(self):
        if not config["test_enable"] or config["test_enable"] and self.env.context.get("_testing_base_tier_validationwaiting"):
            self.status = "waiting"
        else:
            self.status = "pending"
        
    status = fields.Selection(
        selection_add=[("waiting", "Waiting")],
        default="_default_status",
        ondelete={"waiting": "set default"},
    )

Maybe this helps to work with the other module

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch from 2ee9c17 to cd6ccb0 Compare November 22, 2023 20:21
Copy link

@Dranyel-Bosd Dranyel-Bosd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch 2 times, most recently from 902d294 to 4a4b26f Compare November 29, 2023 20:01
@bosd bosd force-pushed the 15.0-mig-base_tier_validation_waiting branch from 4a4b26f to 174337e Compare November 29, 2023 20:07
@bosd
Copy link
Contributor Author

bosd commented Nov 29, 2023

Dropped the dependency on 732. Now we finally have green tests ✔️
✨ 🎉

@bosd bosd requested a review from simahawk November 30, 2023 08:29


class TierReview(models.Model):
_inherit = "tier.review"

def _default_status(self):
if (
not config["test_enable"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this specific check for tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, without this commit, it would mean that we need to patch a lot of depending modules, to fix their tests.

Another solution I've evaluated is using seperate tests. That might solve the problem with the CI tests here on github.
But it would simply transfer the problem to the persons running an custom CI like odoo.sh.

The method here is not really clean. But it's the least dirty solution.
(Discussed this test issue with a couple of oca contributors at the OXP)
Maybe you have another insight to solve this problem?

status = fields.Selection(
selection_add=[("waiting", "Waiting")],
default="waiting",
default=_default_status,
Copy link
Contributor

@simahawk simahawk Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use lambda to not reference the method object directly

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Mar 31, 2024
@github-actions github-actions bot closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants